refactor (backup) : conditionally copy registry auth secret from operator namespace to workspace namespace for backup/restore#1618
Conversation
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughThis PR refactors registry auth secret handling in the backup path. ChangesRegistry Auth Secret Backup-Path Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rohanKanojia The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
rohanKanojia
force-pushed
the
pr/secret-fallback-option
branch
from
56c6857 to
f6f485c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1618 +/- ##
==========================================
+ Coverage 35.48% 36.94% +1.46%
==========================================
Files 168 168
Lines 14484 14726 +242
==========================================
+ Hits 5139 5441 +302
+ Misses 9006 8932 -74
- Partials 339 353 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rohanKanojia
force-pushed
the
pr/secret-fallback-option
branch
3 times, most recently
from
802c09c to
87f720c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
controllers/backupcronjob/backupcronjob_controller_test.go (1)
312-314: ⚡ Quick winAdd one unset/false-path test for
CopyOperatorAuthSecret.All touched
executeBackupSyncfixtures now forceCopyOperatorAuthSecret=true, so this suite no longer validates the new default/disabled branch. Please add at least one case for unset/false (especially missing workspace secret) to guard the new behavior.Also applies to: 345-346, 403-404, 440-441, 482-483, 514-516, 551-553, 588-589
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/backupcronjob/backupcronjob_controller_test.go` around lines 312 - 314, Tests in backupcronjob_controller_test.go currently set CopyOperatorAuthSecret = pointer.Bool(true) in all executeBackupSync fixtures, so the default/disabled branch (unset/false) and the missing workspace secret path are not covered; add at least one test case for executeBackupSync where CopyOperatorAuthSecret is either nil (unset) or pointer.Bool(false) and the workspace secret is absent, then assert the controller follows the disabled behavior (e.g., does not attempt to copy operator auth and returns the expected status/condition). Update the relevant table/fixtures used by TestExecuteBackupSync (or similarly named test harness) to include this false/unset case and verify expected outcomes for missing workspace secret to guard the new branch.deploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yaml (1)
222-240: ⚡ Quick winSet an explicit CRD default for
copyOperatorAuthSecret.Line 239 says the field defaults to false, but the schema currently has no
default: false. Adding it makes the API contract enforceable and avoids nil/merge ambiguity when the field is omitted.Suggested schema tweak
copyOperatorAuthSecret: + default: false description: |- CopyOperatorAuthSecret controls whether the operator should copy the authentication secret from the operator namespace to the workspace namespace when it's not found in the workspace namespace.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yaml` around lines 222 - 240, The CRD schema for the boolean field copyOperatorAuthSecret currently documents "Defaults to false" but lacks an explicit default; update the OpenAPI schema for the field copyOperatorAuthSecret (the property under the spec schema) to include default: false so the CRD enforces the default value when the field is omitted, ensuring API server/merging behavior matches the documented contract.pkg/secrets/backup.go (2)
101-176: 💤 Low valueLogic and race handling look correct.
EnsureRegistryAuthSecretcorrectly: (1) never overwrites a user-provided secret, (2) returns a clear, actionable error whencopyOperatorAuthSecretis false, (3) handles the Get→Create race viaIsAlreadyExistsre-fetch, and (4) sets the watch label and controller reference on the created secret.Two minor observations (no action required, just flagging for awareness):
- The relevant code snippet from
pkg/provision/sync/sync.goshowsSyncObjectWithClusterprovides diff/update behavior. Bypassing it here is the right call since the explicit goal is to never overwrite — just make sure no future caller expects this function to reconcile drift in the workspace secret (e.g., if the operator's source secret rotates, the workspace copy will not be updated).- The error message at lines 125–134 references
DevWorkspaceBackupAuthSecretName(the predefined name) for both the "not found" name and the "please create" name. That's consistent with the rest of the function but means the user-facing message will not mention the configuredsecretNamefromRegistry.AuthSecret, which can be confusing if the admin configured a custom name. Consider including both names in the error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup.go` around lines 101 - 176, Update the error returned when copyOperatorAuthSecret is false in EnsureRegistryAuthSecret to include both the workspace secret name (constants.DevWorkspaceBackupAuthSecretName) and the configured/operator secret name (use sourceSecret.Name) so users see which configured auth secret name the operator expects versus the name missing in the workspace; change the fmt.Errorf in the !copyOperatorAuthSecret branch inside EnsureRegistryAuthSecret to format and include sourceSecret.Name alongside constants.DevWorkspaceBackupAuthSecretName.
30-30: ⚡ Quick winReplace deprecated
k8s.io/utils/pointerwithk8s.io/utils/ptr.
golangci-lint(SA1019) reports thatk8s.io/utils/pointeris deprecated. Useptr.Derefinstead ofpointer.BoolDeref.♻️ Proposed fix
- "k8s.io/utils/pointer" + "k8s.io/utils/ptr"- copyOperatorAuthSecret := pointer.BoolDeref( - dwOperatorConfig.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret, - false, - ) + copyOperatorAuthSecret := ptr.Deref( + dwOperatorConfig.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret, + false, + )k8s.io/utils/ptr Deref function signature🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup.go` at line 30, Replace the deprecated import and calls to pointer.*: change the import "k8s.io/utils/pointer" to "k8s.io/utils/ptr" and update call sites using pointer.BoolDeref (and any other pointer.XDeref) to use ptr.Deref instead, e.g. replace pointer.BoolDeref(someBoolPtr, false) with ptr.Deref(someBoolPtr, false) in the functions/methods in pkg/secrets/backup.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@deploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml`:
- Around line 226-244: The CRD documents that copyOperatorAuthSecret defaults to
false but the schema lacks an enforced default; add a JSON schema default by
setting default: false for the copyOperatorAuthSecret property in the CRD (the
YAML under copyOperatorAuthSecret) so the API server will apply the default. If
the CRD is generated from Go types, add the kubebuilder default marker (e.g., //
+kubebuilder:default=false) to the Go field named CopyOperatorAuthSecret (or
copyOperatorAuthSecret) in the API type, regenerate the CRD manifests, and
verify the generated CRD includes default: false under the
copyOperatorAuthSecret schema.
In `@deploy/deployment/openshift/combined.yaml`:
- Around line 226-244: The CRD OpenAPI schema for the copyOperatorAuthSecret
boolean lacks an explicit default; add "default: false" to the property
definition in the combined.yaml CRD schema so the OpenAPIv3 spec matches the
documented and Go behavior (pointer.BoolDeref(..., false)); also update any CRD
description if needed to note this change is a breaking behavioral change for
users who relied on true.
In
`@deploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml`:
- Around line 223-224: The sentence about the
"controller.devfile.io/watch-secret=true" label is ambiguous: update the text
that references authSecret to state this label requirement only applies to
operator-managed (copied) secrets when copyOperatorAuthSecret is enabled (i.e.,
the operator-created/copy-source path), and clarify that user-provided/manual
authSecret values do not need that label; reference the fields authSecret and
copyOperatorAuthSecret and explicitly mention "operator-managed/copy-source
secrets" and "user-provided/manual secrets" so readers know which secrets must
have controller.devfile.io/watch-secret=true.
In
`@deploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yaml`:
- Around line 224-242: The CRD schema for the boolean property
copyOperatorAuthSecret declares "Defaults to false" in the description but lacks
an OpenAPI default; update the CRD schema for the copyOperatorAuthSecret
property by adding default: false alongside its type: boolean (i.e., under the
copyOperatorAuthSecret property in the CRD's schema) so the generated API docs
and schema reflect the described default behavior.
In `@pkg/secrets/backup.go`:
- Around line 57-61: The change introduced a breaking default (currently using
pointer.BoolDeref(..., false)) for
dwOperatorConfig.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret which
stops auto-copying the operator namespace secret; restore backward-compatible
behavior by changing the default to true in the copyOperatorAuthSecret
initialization (i.e., use pointer.BoolDeref(..., true)) so existing clusters
continue to auto-copy, and if you intentionally want false instead, update
docs/changelog and tests that rely on auto-copy (e.g.,
controllers/backupcronjob/backupcronjob_controller_test.go) to avoid upgrade
surprises.
---
Nitpick comments:
In `@controllers/backupcronjob/backupcronjob_controller_test.go`:
- Around line 312-314: Tests in backupcronjob_controller_test.go currently set
CopyOperatorAuthSecret = pointer.Bool(true) in all executeBackupSync fixtures,
so the default/disabled branch (unset/false) and the missing workspace secret
path are not covered; add at least one test case for executeBackupSync where
CopyOperatorAuthSecret is either nil (unset) or pointer.Bool(false) and the
workspace secret is absent, then assert the controller follows the disabled
behavior (e.g., does not attempt to copy operator auth and returns the expected
status/condition). Update the relevant table/fixtures used by
TestExecuteBackupSync (or similarly named test harness) to include this
false/unset case and verify expected outcomes for missing workspace secret to
guard the new branch.
In
`@deploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yaml`:
- Around line 222-240: The CRD schema for the boolean field
copyOperatorAuthSecret currently documents "Defaults to false" but lacks an
explicit default; update the OpenAPI schema for the field copyOperatorAuthSecret
(the property under the spec schema) to include default: false so the CRD
enforces the default value when the field is omitted, ensuring API
server/merging behavior matches the documented contract.
In `@pkg/secrets/backup.go`:
- Around line 101-176: Update the error returned when copyOperatorAuthSecret is
false in EnsureRegistryAuthSecret to include both the workspace secret name
(constants.DevWorkspaceBackupAuthSecretName) and the configured/operator secret
name (use sourceSecret.Name) so users see which configured auth secret name the
operator expects versus the name missing in the workspace; change the fmt.Errorf
in the !copyOperatorAuthSecret branch inside EnsureRegistryAuthSecret to format
and include sourceSecret.Name alongside
constants.DevWorkspaceBackupAuthSecretName.
- Line 30: Replace the deprecated import and calls to pointer.*: change the
import "k8s.io/utils/pointer" to "k8s.io/utils/ptr" and update call sites using
pointer.BoolDeref (and any other pointer.XDeref) to use ptr.Deref instead, e.g.
replace pointer.BoolDeref(someBoolPtr, false) with ptr.Deref(someBoolPtr, false)
in the functions/methods in pkg/secrets/backup.go.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58f3b582-37ca-446d-9ace-6bf37dec79a5
📒 Files selected for processing (12)
apis/controller/v1alpha1/devworkspaceoperatorconfig_types.goapis/controller/v1alpha1/zz_generated.deepcopy.gocontrollers/backupcronjob/backupcronjob_controller_test.godeploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yamldeploy/deployment/kubernetes/combined.yamldeploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yamldeploy/deployment/openshift/combined.yamldeploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yamldeploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yamlpkg/config/sync.gopkg/secrets/backup.gopkg/secrets/backup_test.go
| The secret must contain "controller.devfile.io/watch-secret=true" label so that it can be | ||
| recognized by the operator. |
There was a problem hiding this comment.
Clarify label requirement scope for user-provided secrets
Line 223 currently reads as if all authSecret values must have controller.devfile.io/watch-secret=true. That conflicts with the manual-secret path when copyOperatorAuthSecret is disabled. Please scope this sentence to operator-managed/copy-source secrets only.
✏️ Proposed wording tweak
- The secret must contain "controller.devfile.io/watch-secret=true" label so that it can be
- recognized by the operator.
+ For operator-managed secret copying, the source secret in the operator namespace must contain
+ "controller.devfile.io/watch-secret=true" so the operator can recognize and manage it.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The secret must contain "controller.devfile.io/watch-secret=true" label so that it can be | |
| recognized by the operator. | |
| For operator-managed secret copying, the source secret in the operator namespace must contain | |
| "controller.devfile.io/watch-secret=true" so the operator can recognize and manage it. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@deploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml`
around lines 223 - 224, The sentence about the
"controller.devfile.io/watch-secret=true" label is ambiguous: update the text
that references authSecret to state this label requirement only applies to
operator-managed (copied) secrets when copyOperatorAuthSecret is enabled (i.e.,
the operator-created/copy-source path), and clarify that user-provided/manual
authSecret values do not need that label; reference the fields authSecret and
copyOperatorAuthSecret and explicitly mention "operator-managed/copy-source
secrets" and "user-provided/manual secrets" so readers know which secrets must
have controller.devfile.io/watch-secret=true.
87f720c to
0f8ddda
Compare
| // If true: The operator will copy the secret from the operator namespace | ||
| // if it's not found in the workspace namespace. This provides automatic configuration | ||
| // but exposes operator-level credentials to workspace users. |
There was a problem hiding this comment.
I think it's a bit verbose,
Can we update thee If true... section to:
If true, this enables the fallback mechanism where the operator will copy the secret from the operator namespace.
| // | ||
| // If false (default): The operator will not copy the secret. Users must manually create a secret | ||
| // with the configured name in their workspace namespace. This is more secure as it allows | ||
| // users to provide scoped credentials with minimal privileges. |
There was a problem hiding this comment.
I think it's a bit verbose,
Can we update the If false... section to:
If false, the operator will not copy the secret to workspace namespaces.
| // users to provide scoped credentials with minimal privileges. | ||
| // | ||
| // Note: Regardless of this setting, if a secret already exists in the workspace namespace, | ||
| // it will never be overwritten. User-provided secrets are always respected. |
There was a problem hiding this comment.
| // it will never be overwritten. User-provided secrets are always respected. | |
| // it will never be overwritten. |
| // Extract flag value (default: false, users must provide their own secret) | ||
| copyOperatorAuthSecret := pointer.BoolDeref( | ||
| dwOperatorConfig.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret, | ||
| false, |
There was a problem hiding this comment.
Could we instead define the default field here?
There was a problem hiding this comment.
Initially, I also thought about it, but then hesitated as RegistryConfig wasn't defined in defaults.go as Registry.Path is a required value with no sensible default.
If we add configuration like this for the default value:
BackupCronJob: &v1alpha1.BackupCronJobConfig{
Enable: pointer.Bool(false),
Schedule: "0 0 1 * *",
BackoffLimit: pointer.Int32(1),
Registry: &v1alpha1.RegistryConfig{
Path: "", // Required but no default
AuthSecret: "",
CopyOperatorAuthSecret: pointer.Bool(false),
},
},But this would generate an incomplete/invalid configuration.
Do you think I should proceed with this approach? Or create a named constant for false value in the current line?
| return nil, fmt.Errorf( | ||
| "registry auth secret %q not found in workspace namespace %q. "+ | ||
| "copyOperatorAuthSecret is set to false, so the operator will not copy the secret. "+ | ||
| "Please manually create a secret of type kubernetes.io/dockerconfigjson with the name %q "+ |
There was a problem hiding this comment.
| "Please manually create a secret of type kubernetes.io/dockerconfigjson with the name %q "+ | |
| "Please create a secret of type kubernetes.io/dockerconfigjson with the name %q "+ |
| if !copyOperatorAuthSecret { | ||
| return nil, fmt.Errorf( | ||
| "registry auth secret %q not found in workspace namespace %q. "+ | ||
| "copyOperatorAuthSecret is set to false, so the operator will not copy the secret. "+ |
There was a problem hiding this comment.
| "copyOperatorAuthSecret is set to false, so the operator will not copy the secret. "+ | |
| "copyOperatorAuthSecret is set to false, the secret will not be copied. "+ |
|
@rohanKanojia on second thought, is there ever a use case where the admin sets an Because, the only purpose of the admin supplying the Also in general the secret name provided in |
rohanKanojia
force-pushed
the
pr/secret-fallback-option
branch
from
0f8ddda to
43f4862
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/secrets/backup.go (1)
63-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
AuthSecretname is silently ignored on the workspace-namespace fallback path — confirm UX.The function uses two different names depending on the path:
- Backup path (line 66,
lookupName = secretName): the workspace namespace is searched under the operator-configuredAuthSecretname.- Restore path (line 68) and the
EnsureRegistryAuthSecretwrite path (line 108): the predefinedconstants.DevWorkspaceBackupAuthSecretNameis used.So when an admin sets
authSecret: my-secretandcopyOperatorAuthSecret: false, and the workspace user has not pre-created any secret, the workspace lookup at line 71–79 will succeed only if they createdmy-secret, butEnsureRegistryAuthSecret(and the error it returns at lines 125–134) instructs them to createdevworkspace-backup-registry-auth. The two paths disagree on the expected name.Please align the names — either always look for/instruct on
constants.DevWorkspaceBackupAuthSecretName, or always honor the operator-configuredAuthSecretname in both lookup and the user-facing error. This was also raised in PR review feedback by dkwon17.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup.go` around lines 63 - 98, The code currently looks up the workspace secret using secretName but EnsureRegistryAuthSecret (and its error/log messages) uses constants.DevWorkspaceBackupAuthSecretName, causing a mismatch; fix by making the operator-configured AuthSecret name authoritative: update EnsureRegistryAuthSecret (and any user-facing error/log text) to use the passed-in secretName instead of constants.DevWorkspaceBackupAuthSecretName so the lookupName logic, the Get calls, and the EnsureRegistryAuthSecret write/error messaging all reference the same secretName; ensure references to lookupName, secretName, and EnsureRegistryAuthSecret are consistent and adjust tests/logs accordingly.
♻️ Duplicate comments (1)
deploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml (1)
221-224:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winScope the watch-secret label requirement to operator-managed copy-source secrets only.
The label sentence is still ambiguous and reads as if it applies to all
authSecretvalues. Please explicitly distinguish operator-managed/copy-source secrets vs user-provided/manual workspace secrets.✏️ Suggested doc tweak
- If secret is not found in the workspace namespace and copyOperatorAuthSecret is true, - the operator will copy the secret from the operator namespace to the workspace namespace. - The secret must contain "controller.devfile.io/watch-secret=true" label so that it can be - recognized by the operator. + If secret is not found in the workspace namespace and copyOperatorAuthSecret is true, + the operator will copy the secret from the operator namespace to the workspace namespace. + For operator-managed/copy-source secrets, the source secret in the operator namespace must + contain "controller.devfile.io/watch-secret=true" so it can be recognized by the operator. + User-provided/manual secrets in workspace namespaces do not require this label.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml` around lines 221 - 224, Clarify that the "controller.devfile.io/watch-secret=true" label is only required for operator-managed copy-source secrets when copyOperatorAuthSecret is true: update the description around the authSecret/copyOperatorAuthSecret text to state that if copyOperatorAuthSecret is true the operator will copy the secret from the operator namespace and that only those copy-source secrets must carry the controller.devfile.io/watch-secret=true label; explicitly note that user-provided/manual workspace secrets (i.e., when users set authSecret directly in the workspace namespace) do not need this label. Reference copyOperatorAuthSecret and authSecret and the watch-secret label in the updated sentence so it's unambiguous.
🧹 Nitpick comments (2)
pkg/secrets/backup.go (2)
87-87: 💤 Low valueLog key/value mismatch on the restore path.
lookupNamewas used for the workspace-namespaceGet(line 73), but this log line reportssecretNameas the secret it failed to find in the workspace namespace. On the restore path these can differ (line 68 overrideslookupNameto the predefined constant), so the log will misattribute the missing key. UselookupNamehere.♻️ Proposed fix
- log.Info("Registry auth secret not found in workspace namespace, checking operator namespace", "secretName", secretName) + log.Info("Registry auth secret not found in workspace namespace, checking operator namespace", + "workspaceSecretName", lookupName, "operatorSecretName", secretName)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup.go` at line 87, The log message in pkg/secrets/backup.go incorrectly reports "secretName" when the code uses lookupName for the workspace-namespace Get; update the log.Info call that currently logs "secretName" to instead log the lookupName variable so the message reflects the actual attempted key (refer to the lookupName variable and the log.Info call that says "Registry auth secret not found in workspace namespace, checking operator namespace").
27-30: 💤 Low valueSwitch from deprecated
k8s.io/utils/pointertok8s.io/utils/ptr.The
pointerpackage is deprecated in favor ofptr. Useptr.Deref(replacingpointer.BoolDeref) to align with Kubernetes project migration guidance.♻️ Proposed refactor
- "k8s.io/utils/pointer" + "k8s.io/utils/ptr"- copyOperatorAuthSecret := pointer.BoolDeref( - dwOperatorConfig.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret, - false, - ) + copyOperatorAuthSecret := ptr.Deref( + dwOperatorConfig.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret, + false, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup.go` around lines 27 - 30, The code imports the deprecated package alias pointer and uses pointer.BoolDeref; replace the import "k8s.io/utils/pointer" with "k8s.io/utils/ptr" (alias it as ptr if you used pointer) and update usages of pointer.BoolDeref to ptr.Deref (e.g., pointer.BoolDeref(someBoolPtr, false) -> ptr.Deref(someBoolPtr, false)); also change any other pointer.* helpers to their ptr.* equivalents (e.g., pointer.Bool -> ptr.Bool) so the file uses the non-deprecated ptr package and ptr.Deref consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/secrets/backup.go`:
- Around line 125-134: The user-facing error string in the fmt.Errorf return
(the block referencing constants.DevWorkspaceBackupAuthSecretName and
workspace.Namespace in the copyOperatorAuthSecret branch) concatenates sentences
without a space resulting in "...will not be copied.Please create..."; fix it by
inserting a space between the period and "Please" (or restructure the string
concatenation so sentences are separated with a space) so the message reads
"...will not be copied. Please create...".
---
Outside diff comments:
In `@pkg/secrets/backup.go`:
- Around line 63-98: The code currently looks up the workspace secret using
secretName but EnsureRegistryAuthSecret (and its error/log messages) uses
constants.DevWorkspaceBackupAuthSecretName, causing a mismatch; fix by making
the operator-configured AuthSecret name authoritative: update
EnsureRegistryAuthSecret (and any user-facing error/log text) to use the
passed-in secretName instead of constants.DevWorkspaceBackupAuthSecretName so
the lookupName logic, the Get calls, and the EnsureRegistryAuthSecret
write/error messaging all reference the same secretName; ensure references to
lookupName, secretName, and EnsureRegistryAuthSecret are consistent and adjust
tests/logs accordingly.
---
Duplicate comments:
In
`@deploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml`:
- Around line 221-224: Clarify that the
"controller.devfile.io/watch-secret=true" label is only required for
operator-managed copy-source secrets when copyOperatorAuthSecret is true: update
the description around the authSecret/copyOperatorAuthSecret text to state that
if copyOperatorAuthSecret is true the operator will copy the secret from the
operator namespace and that only those copy-source secrets must carry the
controller.devfile.io/watch-secret=true label; explicitly note that
user-provided/manual workspace secrets (i.e., when users set authSecret directly
in the workspace namespace) do not need this label. Reference
copyOperatorAuthSecret and authSecret and the watch-secret label in the updated
sentence so it's unambiguous.
---
Nitpick comments:
In `@pkg/secrets/backup.go`:
- Line 87: The log message in pkg/secrets/backup.go incorrectly reports
"secretName" when the code uses lookupName for the workspace-namespace Get;
update the log.Info call that currently logs "secretName" to instead log the
lookupName variable so the message reflects the actual attempted key (refer to
the lookupName variable and the log.Info call that says "Registry auth secret
not found in workspace namespace, checking operator namespace").
- Around line 27-30: The code imports the deprecated package alias pointer and
uses pointer.BoolDeref; replace the import "k8s.io/utils/pointer" with
"k8s.io/utils/ptr" (alias it as ptr if you used pointer) and update usages of
pointer.BoolDeref to ptr.Deref (e.g., pointer.BoolDeref(someBoolPtr, false) ->
ptr.Deref(someBoolPtr, false)); also change any other pointer.* helpers to their
ptr.* equivalents (e.g., pointer.Bool -> ptr.Bool) so the file uses the
non-deprecated ptr package and ptr.Deref consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a377f982-ce2d-4827-926d-c2adb24e358e
📒 Files selected for processing (12)
apis/controller/v1alpha1/devworkspaceoperatorconfig_types.goapis/controller/v1alpha1/zz_generated.deepcopy.gocontrollers/backupcronjob/backupcronjob_controller_test.godeploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yamldeploy/deployment/kubernetes/combined.yamldeploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yamldeploy/deployment/openshift/combined.yamldeploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yamldeploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yamlpkg/config/sync.gopkg/secrets/backup.gopkg/secrets/backup_test.go
✅ Files skipped from review due to trivial changes (1)
- deploy/deployment/kubernetes/combined.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/config/sync.go
- apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go
- deploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml
- deploy/deployment/openshift/combined.yaml
- deploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yaml
- pkg/secrets/backup_test.go
- deploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yaml
- apis/controller/v1alpha1/zz_generated.deepcopy.go
rohanKanojia
force-pushed
the
pr/secret-fallback-option
branch
from
18bf441 to
385a1b3
Compare
|
@rohanKanojia: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
rohanKanojia
force-pushed
the
pr/secret-fallback-option
branch
from
385a1b3 to
8afc9dc
Compare
rohanKanojia
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/secrets/backup.go (2)
34-40: ⚡ Quick winStale doc comment references the old function name.
The godoc on line 34 still says
GetRegistryAuthSecretwhile the function is nowGetNamespaceRegistryAuthSecret. Please align the comment with the new name (and ideally describe what "Namespace" means here since this thin wrapper just callsHandleRegistryAuthSecretwith an emptyoperatorConfigNamespace).📝 Suggested doc fix
-// GetRegistryAuthSecret retrieves the registry authentication secret for accessing backup images -// based on the operator configuration. +// GetNamespaceRegistryAuthSecret retrieves the registry authentication secret from the workspace +// namespace only (no fallback to the operator namespace). It is intended for the restore path where +// only user-provided, workspace-scoped credentials should be used. func GetNamespaceRegistryAuthSecret(ctx context.Context, c client.Client, workspace *dw.DevWorkspace,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup.go` around lines 34 - 40, Update the stale godoc for GetNamespaceRegistryAuthSecret to reference its current name (replace "GetRegistryAuthSecret" with "GetNamespaceRegistryAuthSecret") and clarify that this wrapper calls HandleRegistryAuthSecret with an empty operatorConfigNamespace to use the default/operator-scoped config; mention what "Namespace" refers to (i.e., it operates for the workspace namespace by passing an empty operatorConfigNamespace). Ensure the comment sits immediately above the GetNamespaceRegistryAuthSecret function signature.
94-152: ⚡ Quick win
CopySecretis now narrowly purpose-built for the backup-auth secret; consider renaming/narrowing the signature.The function still has the generic name
CopySecret(... sourceSecret *corev1.Secret ...), but it ignoressourceSecret.Name/sourceSecret.Namespaceand hardcodes the destination name toconstants.DevWorkspaceBackupAuthSecretName. A future caller passingsourceSecretnamed "foo" would silently end up with a workspace secret nameddevworkspace-backup-registry-auth, which is surprising.Two reasonable options:
- Rename to something like
EnsureBackupAuthSecretInWorkspaceand shrink the signature to just the bits actually consumed (data map[string][]byte,secretType corev1.SecretType), making it explicit that this is not a general-purpose copy.- Keep the generic name but actually honor
sourceSecret.Name(and require it to equal the constant when called fromHandleRegistryAuthSecret), so the function stays reusable.Also note: this nicely pairs with the previous comment — if
Registry.AuthSecretbecomes the operator-namespace source name, option (1) becomes the cleaner separation, since the source/destination naming differ.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup.go` around lines 94 - 152, The CopySecret function is actually specialized for the backup-auth secret but still takes a generic sourceSecret and ignores its name; rename CopySecret to EnsureBackupAuthSecretInWorkspace (or similar) and shrink its signature to only accept the fields it uses (e.g., data map[string][]byte, secretType corev1.SecretType, workspace *dw.DevWorkspace, scheme *runtime.Scheme, ctx/client/log) so callers can't accidentally expect a generic copy; update all callers (e.g., HandleRegistryAuthSecret) to pass sourceSecret.Data and sourceSecret.Type (or keep a thin adapter if needed), keep the hardcoded destination name constants.DevWorkspaceBackupAuthSecretName and the existing controllerutil.SetControllerReference, and preserve the existing Get/Create/IsAlreadyExists race handling and logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/secrets/backup.go`:
- Around line 67-89: The code currently ignores the admin-configured name in
dwOperatorConfig.Workspace.BackupCronJob.Registry.AuthSecret and always looks up
constants.DevWorkspaceBackupAuthSecretName in the operator namespace; change the
operator-namespace lookup (the c.Get call that populates registryAuthSecret) to
use the configured name
(dwOperatorConfig.Workspace.BackupCronJob.Registry.AuthSecret) instead of the
constant, and update the preceding error/log message to reflect that the
configured name will be used for the operator-namespace lookup; also update
related tests (backup_test.go) to seed the operator namespace with the
configured secret name rather than constants.DevWorkspaceBackupAuthSecretName.
---
Nitpick comments:
In `@pkg/secrets/backup.go`:
- Around line 34-40: Update the stale godoc for GetNamespaceRegistryAuthSecret
to reference its current name (replace "GetRegistryAuthSecret" with
"GetNamespaceRegistryAuthSecret") and clarify that this wrapper calls
HandleRegistryAuthSecret with an empty operatorConfigNamespace to use the
default/operator-scoped config; mention what "Namespace" refers to (i.e., it
operates for the workspace namespace by passing an empty
operatorConfigNamespace). Ensure the comment sits immediately above the
GetNamespaceRegistryAuthSecret function signature.
- Around line 94-152: The CopySecret function is actually specialized for the
backup-auth secret but still takes a generic sourceSecret and ignores its name;
rename CopySecret to EnsureBackupAuthSecretInWorkspace (or similar) and shrink
its signature to only accept the fields it uses (e.g., data map[string][]byte,
secretType corev1.SecretType, workspace *dw.DevWorkspace, scheme
*runtime.Scheme, ctx/client/log) so callers can't accidentally expect a generic
copy; update all callers (e.g., HandleRegistryAuthSecret) to pass
sourceSecret.Data and sourceSecret.Type (or keep a thin adapter if needed), keep
the hardcoded destination name constants.DevWorkspaceBackupAuthSecretName and
the existing controllerutil.SetControllerReference, and preserve the existing
Get/Create/IsAlreadyExists race handling and logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48f686d6-3831-4b20-8a0e-357bc50c82c8
📒 Files selected for processing (2)
pkg/secrets/backup.gopkg/secrets/backup_test.go
|
|
||
| // Check if AuthSecret is configured in operator config | ||
| if len(dwOperatorConfig.Workspace.BackupCronJob.Registry.AuthSecret) == 0 { | ||
| return nil, fmt.Errorf( | ||
| "registry auth secret %q not found in workspace namespace %q and AuthSecret is not configured in DevWorkspaceOperatorConfig. "+ | ||
| "Either create the secret in the workspace namespace or configure Registry.AuthSecret in the operator config to enable copying from the operator namespace", | ||
| constants.DevWorkspaceBackupAuthSecretName, | ||
| workspace.Namespace, | ||
| ) | ||
| } | ||
|
|
||
| log.Info("Registry auth secret not found in workspace namespace, checking operator namespace", | ||
| "secretName", constants.DevWorkspaceBackupAuthSecretName, | ||
| "operatorNamespace", operatorConfigNamespace) | ||
|
|
||
| // If the secret is not found in the workspace namespace, check the operator namespace as fallback | ||
| err = c.Get(ctx, client.ObjectKey{ | ||
| Name: secretName, | ||
| Name: constants.DevWorkspaceBackupAuthSecretName, | ||
| Namespace: operatorConfigNamespace}, registryAuthSecret) | ||
| if err != nil { | ||
| log.Error(err, "Failed to get registry auth secret for backup job", "secretName", secretName) | ||
| log.Error(err, "Failed to get registry auth secret for backup job", "secretName", constants.DevWorkspaceBackupAuthSecretName) | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Admin-configured Registry.AuthSecret name is silently ignored.
The gate at line 69 only checks whether Registry.AuthSecret is non-empty, but the actual operator-namespace lookup at line 84 uses the hard-coded constants.DevWorkspaceBackupAuthSecretName. Net effect: an admin who configures AuthSecret: "my-org-pull-secret" will hit a confusing NotFound for devworkspace-backup-registry-auth in the operator namespace, with no hint that the configured name was disregarded. This matches the concern raised on the PR that the configured AuthSecret name is effectively ignored by the lookup logic.
There are also two contradictory readings of the same field in this code path:
- The error string at line 71-72 tells admins to "configure Registry.AuthSecret in the operator config to enable copying", treating it as a boolean.
- The CRD/type defines
AuthSecretas a name (string), so admins reasonably expect it to be the name used for the lookup.
Pick one model and stick to it:
- Treat
AuthSecretas a name (preferred, matches the field's existing semantics): use it for the operator-namespaceGet, and only fall through if it is set. - Use a dedicated boolean (
copyOperatorAuthSecretmentioned in the PR description) for the toggle, and keepAuthSecretas the source name in the operator namespace.
Either way, the configured value should not be silently dropped.
🛠️ Sketch for option 1 (use AuthSecret as the operator-namespace name)
- // Check if AuthSecret is configured in operator config
- if len(dwOperatorConfig.Workspace.BackupCronJob.Registry.AuthSecret) == 0 {
+ operatorAuthSecretName := dwOperatorConfig.Workspace.BackupCronJob.Registry.AuthSecret
+ if operatorAuthSecretName == "" {
return nil, fmt.Errorf(
"registry auth secret %q not found in workspace namespace %q and AuthSecret is not configured in DevWorkspaceOperatorConfig. "+
"Either create the secret in the workspace namespace or configure Registry.AuthSecret in the operator config to enable copying from the operator namespace",
constants.DevWorkspaceBackupAuthSecretName,
workspace.Namespace,
)
}
log.Info("Registry auth secret not found in workspace namespace, checking operator namespace",
- "secretName", constants.DevWorkspaceBackupAuthSecretName,
+ "secretName", operatorAuthSecretName,
"operatorNamespace", operatorConfigNamespace)
// If the secret is not found in the workspace namespace, check the operator namespace as fallback
err = c.Get(ctx, client.ObjectKey{
- Name: constants.DevWorkspaceBackupAuthSecretName,
+ Name: operatorAuthSecretName,
Namespace: operatorConfigNamespace}, registryAuthSecret)The corresponding tests in backup_test.go (e.g. lines 193-217, 248-258) would need to seed/reference operatorAuthSecretName rather than constants.DevWorkspaceBackupAuthSecretName in the operator namespace.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/secrets/backup.go` around lines 67 - 89, The code currently ignores the
admin-configured name in
dwOperatorConfig.Workspace.BackupCronJob.Registry.AuthSecret and always looks up
constants.DevWorkspaceBackupAuthSecretName in the operator namespace; change the
operator-namespace lookup (the c.Get call that populates registryAuthSecret) to
use the configured name
(dwOperatorConfig.Workspace.BackupCronJob.Registry.AuthSecret) instead of the
constant, and update the preceding error/log message to reflect that the
configured name will be used for the operator-namespace lookup; also update
related tests (backup_test.go) to seed the operator namespace with the
configured secret name rather than constants.DevWorkspaceBackupAuthSecretName.
|
Hi! I'm che-ai-assistant — I help with your pull requests. I check for new commands every 5m0s (if I am not busy :) ). Available commands:
|
…ator namespace to workspace namespace for backup/restore Only copy the registry auth secret from the operator namespace to the workspace namespace when it is configured inside DevWorkspaceOperatorConfig. Always give precedence to the user-defined secret named devworkspace-backup-registry-auth in the workspace namespace; do not modify it when it's present while copying. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Rohan Kumar <rohaan@redhat.com>
rohanKanojia
force-pushed
the
pr/secret-fallback-option
branch
from
8afc9dc to
3cec08a
Compare
|
/che-ai-assistant generate-che-doc |
3 participants
What does this PR do?
Only copy the registry auth secret from the operator namespace to the workspace namespace when it is configured inside DevWorkspaceOperatorConfig. Always give precedence to the user-defined secret named devworkspace-backup-registry-auth in the workspace namespace; do not modify it when it's present while copying.
What issues does this PR fix or reference?
https://redhat.atlassian.net/browse/CRW-10758
Is it tested? How?
Scenario 1: AuthSecret Not Configured (Secure by Default)
Expected: Operator will NOT copy secrets. User must provide their own.
kubectl patch devworkspace test-workspace-no-copy -n test-workspace-no-auth \ --type merge -p '{"spec":{"started":false}}'Expected log:
kubectl get secret devworkspace-backup-registry-auth -n test-workspace-no-auth # Expected: Error from server (NotFound)Scenario 2: AuthSecret Configured - Operator Copies Secret
Expected: Operator copies secret from operator namespace to workspace namespace.
kubectl patch devworkspace test-workspace-with-copy -n test-workspace-copy \ --type merge -p '{"spec":{"started":false}}'Scenario 3: User-Provided Secret NEVER gets Overwritten
Expected: User's secret is respected and NEVER overwritten, even if operator has different credentials.
kubectl create secret docker-registry quay-push-secret \ --docker-server=quay.io \ --docker-username=operator-user \ --docker-password=operator-token \ -n openshift-operators kubectl label secret quay-push-secret [controller.devfile.io/watch-secret=true](http://controller.devfile.io/watch-secret=true) -n openshift-operatorskubectl patch devworkspace test-workspace-user-secret -n test-workspace-user-secret \ --type merge -p '{"spec":{"started":false}}'Scenario 4: AuthSecret Configured but Operator Secret Missing
Expected: Operator logs error that it cannot find the configured secret in operator namespace.
kubectl patch devworkspace test-workspace-missing -n test-workspace-missing-secret \ --type merge -p '{"spec":{"started":false}}'Expected log:
kubectl get secret devworkspace-backup-registry-auth -n test-workspace-missing-secret # Expected: Error from server (NotFound)PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with CheSummary by CodeRabbit
Bug Fixes
Tests